-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add no_tlsv1_3 to Puma::DSL#ssl_bindings and Puma::MiniSSL::Context #2426
Conversation
Disables TLSv1.3 when front-ends don't support it Also: 1. Remove 'no_tls' false query parameters in bind string 2. Update minissl.c for no_tlsv1_3, whitespace 3. Add test_tls_v1_3_rejection to test_puma_server_ssl.rb 4. Add three protocol tests to test_config.rb 5. extconf.rb - 1.1.0 -> 1.1.1 change (macro to func)
ca_additions = "&ca=#{opts[:ca]}" if ['peer', 'force_peer'].include?(verify) | ||
|
||
if defined?(JRUBY_VERSION) | ||
keystore_additions = "keystore=#{opts[:keystore]}&keystore-pass=#{opts[:keystore_pass]}" | ||
bind "ssl://#{host}:#{port}?cert=#{opts[:cert]}&key=#{opts[:key]}&#{keystore_additions}&verify_mode=#{verify}&no_tlsv1=#{no_tlsv1}&no_tlsv1_1=#{no_tlsv1_1}#{ca_additions}" | ||
bind "ssl://#{host}:#{port}?cert=#{opts[:cert]}&key=#{opts[:key]}&#{keystore_additions}&verify_mode=#{verify}#{tls_str}#{ca_additions}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just realized how silly it is that we pass around arguments to bind
internally as a string rather than as arguments. File that one away for the future...
ctx.no_tlsv1_1 = true if params['no_tlsv1_1'] == 'true' | ||
ctx.no_tlsv1_3 = true if params['no_tlsv1_3'] == 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At what point to we decide there's a more elegant interface here than just disabling individual TLS minor versions? I seem to remember something was discussed a long ways back...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we drop support for OpenSSL 1.0.2? 1.1.1 has min/max functions, with 1.0.2 you have to supply a list of allowed protocols.
JFYI, TLSv1.2 came out in 2008, TLSv1.3 in 2018. What we have now will probably work for a few years...
Issue #1735 pertained to a problem with load balancers (and hence, a high speed connection to Puma), and the question of whether Puma' downward negotiation from TLSv1.3 to TLSv1.2 slowed things down. While working on #2519, and also testing locally with WSL2/Ubuntu, I checked the timing of using TLSv1.2 clients with Puma. Timing both with and without the server restricted to TLSv1.2, there was no significant difference. Hence, closing. |
Description
Disables TLSv1.3 when front-ends don't support it
Also:
See #1735
Your checklist for this pull request
[changelog skip]
or[ci skip]
to the pull request title.[ci skip]
to the title of the PR.#issue
" to the PR description or my commit messages.